-
Notifications
You must be signed in to change notification settings - Fork 537
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
enforce collection id for login parameters #1043
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Reviewed everything up to 1a1ba58 in 15 seconds
More details
- Looked at
42
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. skyvern/forge/sdk/workflow/service.py:961
- Draft comment:
Ensurebitwarden_collection_id
is checked for all relevant parameter types, not justBITWARDEN_LOGIN_CREDENTIAL
. This includesBITWARDEN_SENSITIVE_INFORMATION
andBITWARDEN_CREDIT_CARD_DATA
increate_workflow_from_request
. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_e6wM7d2gi9ZdAzmY
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Changes requested. Incremental review on fc5e561 in 25 seconds
More details
- Looked at
26
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
0
drafted comments based on config settings.
Workflow ID: wflow_YmI5q2l73uo9ZSNy
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
@@ -707,6 +711,8 @@ async def create_workflow( | |||
return await app.WORKFLOW_SERVICE.create_workflow_from_request( | |||
organization=current_org, request=workflow_create_request | |||
) | |||
except WorkflowParameterMissingRequiredValue as e: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Catching WorkflowParameterMissingRequiredValue
and re-raising it is redundant. Consider removing this except
block.
except WorkflowParameterMissingRequiredValue as e: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Changes requested. Incremental review on e9b9cdb in 27 seconds
More details
- Looked at
13
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
0
drafted comments based on config settings.
Workflow ID: wflow_4xbKHqNy7qTEbo4G
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
@@ -753,6 +759,8 @@ async def update_workflow( | |||
request=workflow_create_request, | |||
workflow_permanent_id=workflow_permanent_id, | |||
) | |||
except WorkflowParameterMissingRequiredValue as e: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant exception handling for WorkflowParameterMissingRequiredValue
. The exception is caught and immediately re-raised without any additional processing. Consider removing this except
block.
except WorkflowParameterMissingRequiredValue as e: |
Important
Enforces
bitwarden_collection_id
requirement forBITWARDEN_LOGIN_CREDENTIAL
parameters, raising an exception if missing.bitwarden_collection_id
requirement forBITWARDEN_LOGIN_CREDENTIAL
parameters increate_workflow_from_request()
inservice.py
.WorkflowParameterMissingRequiredValue
ifbitwarden_collection_id
is missing.WorkflowParameterMissingRequiredValue
toexceptions.py
for missing required workflow parameter values.WorkflowParameterMissingRequiredValue
increate_workflow()
andupdate_workflow()
inagent_protocol.py
.This description was created by for e9b9cdb. It will automatically update as commits are pushed.